Conversation
197g
left a comment
197g
left a comment
There was a problem hiding this comment.
Just a question, integer arithmetic with fixed divisors sounds preferable in either case—most of the time at least.
| (((255 - c) * k_inv) / 255) as u8, | ||
| (((255 - m) * k_inv) / 255) as u8, | ||
| (((255 - y) * k_inv) / 255) as u8, |
There was a problem hiding this comment.
It's making a good point that as used floor semantics but wouldn't we prefer rounding here? Then a bias term for 127 should be added here before.
There was a problem hiding this comment.
Thank you! Adding a bias term is mathematically more accurate for rounding, but it again introduced pixel differences when testing against libtiff (which uses truncation by default). I have added another test case for this specific truncation behavior:
CMYK: [1, 1, 1, 128]with intermediate values(255 - c) = 254andk_inv = 127- New logic without bias term:
(254 * 127) / 255 = 32258 / 255 = 126(integer division naturally truncates to 126) - With bias term:
(254 * 127 + 127) / 255 = 32385 / 255 = 127(rounds up to 127)
There was a problem hiding this comment.
@197g Do you think this approach is worth pursuing?
There was a problem hiding this comment.
I'm not thrilled by the truncating behavior but matching the existing widely-used implementation takes priority. So looks good to me.
There was a problem hiding this comment.
Sorry for the later, my head was at some compatibility issues with jpeg and weezl. It's a conflicting issue for me, but overall I tend to agree. Maybe throw in a note at the tests that they are meant to match libtiff first and foremost; not an independent mathematical groundtruth. Behavior is indeed LGTM then.
3 participants
This PR replaces floating-point arithmetic with pure integer arithmetic for CMYK-to-RGB conversion in the TIFF decoder. Fixes Precision Errors:
tiff/testsuite/cmyk_u8_edge_case.tif(edge case with a 1x1 pixel with CMYK [0, 0, 0, 65]) to test this